chore: sampler serialization tests - #250

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.76%. Comparing base (56127d5) to head (d569be8).

@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
- Coverage   93.43%   91.76%   -1.67%     
==========================================
  Files          15       15              
  Lines        1432     1458      +26     
==========================================
  Hits         1338     1338              
- Misses         94      120      +26     

Sorry, one other thing, we also want to make sure that simply making a copy of the samplers works. This is different than pickling

What about #250 (review)?

Did you mean to link this:

What is this testing?

it guards against an ignored/hard-coded rng. Otherwise the tests would still pass

https://docs.python.org/3/library/copy.html#object.__copy__ + https://docs.python.org/3/library/copy.html#object.__deepcopy__

In order to ensure we aren't doing anything funny, it would be probably best to implement copy, deepcopy and eq methods on our samplers

Why? If our attributes don't support copy they would fail anyway. Maybe __eq__ would make sense because you'd define the semantics of equality but I don't see how that would be useful. In the end the implementation would be to recursively copy our attributes but thats already the default implementation

As discussed yesterday, error on __copy__, implement __eq__ and __deepcopy__

The problem with __eq__ is pandas and other structures have their own equals. I did it the hacky way with checking of attribute of equals. Because I didn't want to write every pandas type.

By default python would've compare references. So it would be a stricter check, even though the current function would give somewhat better sense of structural equality if a subclass has an attribute that doesn't implement __eq__, and they forget to override __eq__ on that subclass (which is very likely), they will experience inconsistent behaviour where some codepaths depend on structural-__eq__ for their logic.

To avoid it we'd need

def _attr_equal(a: object, b: object) -> bool:
    if isinstance(a, np.random.Generator) or isinstance(b, np.random.Generator):
        return (isinstance(a, np.random.Generator) and isinstance(b, np.random.Generator)
                and a.bit_generator.state == b.bit_generator.state)
    if isinstance(a, Sampler) or isinstance(b, Sampler):
        return a == b
    if isinstance(a, np.ndarray) or isinstance(b, np.ndarray):
        return isinstance(a, np.ndarray) and isinstance(b, np.ndarray) and bool(np.array_equal(a, b))
    if hasattr(a, "equals") and hasattr(b, "equals") and type(a) is type(b):   # pandas
        return bool(a.equals(b))
    if isinstance(a, int | float | bool | str | bytes | slice | tuple | frozenset | type(None)):
        return type(a) is type(b) and bool(a == b)
    raise TypeError(
        f"Sampler equality doesn't know how to compare {type(a).__name__!r} state. "
        "Extend _attr_equal or override __eq__ on the subclass that added this attribute."
    )

But then this would fail only when __eq__ of unsupported class is being used. Which might be also confusing

I think documenting that the default eq supports numpy, pandas, and Generators is also sufficient, no?

Right this would be the other option to what I'm describing. Add a dumb default, document it, and tell people to override if they need. But blindly taking everything from __dict__ seems error-prone

First of all, nice to see you back :)

One thing I'd like to separate: why do we care about __eq__ in this case? Can't we let it compare the references? Which is more strict anyway?

About the rest I still have questions ,

Maybe this got lost in translation, but I think this should just be implemented custom per-implementation (so you don't have to handle every case as above and what constitutes "equal" is clear for every individual Sampler) - for now, you can make it an optional overload, but warn that in the future, it will become part of the abstract methods required

Okay, I can do that ofc. But again if someone inherits those classes it will break silently (unless we make deepcopy uninheritable for child classes somehow, but do we really want that)?

But blindly taking everything from dict seems error-prone

It is for sure. But I can't think of an unproblematic way to do this.

The best you can do is probably this: we have a registry of classes in which we support and guarantee (ie deepcopyable classes) (it should be a list of classes, the other solution is to have a list of attributes which is super unusual and probably more error prone as well). Under __init_subclass__ we do a check for each field if we recognize it or not. Then give an error telling that class should be recognized somehow explicitly. This also has problems, what if a field is not initialized yet? are we going to do this only by typing hints? etc. etc. Seems like a fight against the unserious nature of python.

Why isn't documenting and proper unit tests not enough here to ensure our guarantees? Like if the pickled bin files are byte identical of samplers, what more can we guarantee?

Okay, I can do that ofc. But again if someone inherits those classes it will break silently (unless we make deepcopy uninheritable for child classes somehow, but do we really want that)?

That was implied yeah, I would be fine making it an abstract method or raising NotImplementedError by default but also having a clearly defined minimal fallback would be fine. I would lean towards the former i.e., forcing people to implement it.

First of all, why do we care about eq in this case? Can't we let it compare the references?

I want to be 100% sure that sampler_1 == sampler_2 does exactly what we want it to, and not something secretly different. For example maybe we want Sampler(...params, gpu_accelerate) == Sampler(...same_params, not_gpu_accelerate) to be True - just a contrived example, but highlights why I'd rather this be explicit.

But I can't think of an unproblematic way to do this.

Just enforcing that people have to implement it?

Why isn't documenting and proper unit tests not enough here to ensure our guarantees?

See above, my idea was that "two samplers are equal if the produce the same iteration output" which is the same as "do they have the same relevant state" but not "do they have the same state completely." Kind of like how np.array(some_number, dtype=np.int32) == np.array(some_number, dtype=np.int64) so I think being explicit about this is good.

That was implied yeah, I would be fine making it an abstract method or raising NotImplementedError by default but also having a clearly defined minimal fallback would be fine. I would lean towards the former i.e., forcing people to implement it.

Sorry I should've been more clear, I meant preventing from inheriting a Sampler's child class''s implementation of deepcopy or __eq__. Do we want that? For example, if someone wants to inherit RandomSampler, should they be required to implement it as well?